Skip to content

Fix of enum warnings for MSVC 2022 ROS2 windows rolling build#2343

Closed
knmcguire wants to merge 1 commit intoeclipse-cyclonedds:releases/0.10.xfrom
knmcguire:fix_enum_warn
Closed

Fix of enum warnings for MSVC 2022 ROS2 windows rolling build#2343
knmcguire wants to merge 1 commit intoeclipse-cyclonedds:releases/0.10.xfrom
knmcguire:fix_enum_warn

Conversation

@knmcguire
Copy link
Copy Markdown

Hi!

This PR is fixing the enum comparison warnings for MSVC 2022 builds for ROS2 on windows (warning C5286 C5287). Cyclone-dds 0.10.x was resulting in over 1500 warnings (See this on the ros2 ci: https://ci.ros2.org/job/test_ci_windows/687/)

With this patch in place, the warnings were resolved (see this test build on ros2 ci).

I'll leave this in draft so that you and the ROS PMC can discuss if this change is desired. An alternative would be to fix this all by all local cmake suppressions of these warnings, but that was a wack-a-mole that required that suppression in a lot of cmake. This was a patch that required the least amount of code change but perhaps not something to be desired.

Related to ros2/ci#832 (comment)

Just let me know what you think.

Generated-by: Claude Code Opus-4.5 (Release 2025/11/01) .
@knmcguire
Copy link
Copy Markdown
Author

We have noticed that this PR together with the master branch of cyclone dds also resolves those warnings. ros2/rmw_cyclonedds#550

I'll close this PR for now, but we can reopen if necessary.

@knmcguire knmcguire closed this Feb 19, 2026
@github-project-automation github-project-automation bot moved this from In Progress to Done in Windows 11 Work! Feb 19, 2026
@knmcguire knmcguire moved this from Done to Skipped in Windows 11 Work! Mar 5, 2026
@knmcguire knmcguire reopened this Mar 20, 2026
@knmcguire knmcguire marked this pull request as ready for review March 20, 2026 13:52
@knmcguire
Copy link
Copy Markdown
Author

just reopened this, but this simple fix will help us out extremely for just ros2 ci to move to MSVC 2022 for the time being. Seems that the other PR just needs more time

Also mind that this is "only" a patch for 0.10.x branch, to which the ros2.repos is pointed towards

@eboasson
Copy link
Copy Markdown
Contributor

It may be "only" a patch to 0.10.x, but:

  • Changing enums to #defines at the minimum causes useful warnings (especially: not all cases covered in a switch) to disappear;
  • I need to spend effort on reviving Cyclone's own CI for 0.10.x because the CI infrastructure moved on;
  • The problem is solved in Cyclone master — and now, at long last, in a release (11.0.1). I'm doing my best to get the PR#550 in;
  • The only problem is that Microsoft introduced a new warning.

Changing the build flags on VS2022 makes more sense to me than changing the code here. The warning level appears to be set at

add_compile_options(/W3)
and one would think that changing it to compile_options(/W3 /wd5286 /wd5287) would also solve the problem. Perhaps you can give that a try?

@knmcguire
Copy link
Copy Markdown
Author

knmcguire commented Mar 20, 2026

Hi!

Ah sorry I should have given a little bit more context and my last comment was perhaps a bit too short and pragmatic. This PR patch is only meant to be very temporary but just for us infra team to get some form of CI going before the lyrical release as we only have a mere few weeks or even less to finalize all work for this.

I'm aware of the other work and PR you are doing and have tried the latest master of cyclonedds (I posted a run of that in PR) and that is meant to be the final solution. This patch is therefore only meant to take the pressure of to get if all merged in time and to enable us to get the ci scaffolding in place just for the time being so that others can hunt down regressions, to be then replaced by cyclone dds' new release and new RMW

We have tried of setting the compile flag for cmake on colcon level but they resulted in build errors, so I had to chase it microlevel cmake lines in a whack a mole fashion (but it's been a while ago so can't remember the details).

Anyway, hope that makes it a bit clear about the intention of this PR

@knmcguire knmcguire marked this pull request as draft March 22, 2026 07:42
@knmcguire
Copy link
Copy Markdown
Author

knmcguire commented Mar 22, 2026

By the way, I did a check with the suppressions you have suggested at that same location. that seems to have worked! Thanks for this!

See this run with suppressions Build Status (only 1 msbuild warning)
Compared to this run without suppressions: Build Status (1800+ msbuild warnings)

I had a faint memory that I did see these suppressions in the cmakelist before in the cyclone dds master (but I can not find it now) and therefore tried something similar as well, but this was a local docker test so I can not find a run of this. At least, I was too quickly to brush it away when the build failed.

Let me try to clear out all warnings and make a new minimal PR with those suppressions. I put this one back in draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants